Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make auto the default for layer config #1016

Merged
merged 25 commits into from
Sep 11, 2024

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented May 30, 2024

Description

This changes config_from_keras in 'name' mode to make the layer precisions to default to 'auto'. (It is generally recommended to pass the backend in that case. The default precision is still provided in the model level and generally is used as the real default when the precision cannot be inferred.

Note: for PTQ, this change could cause the model widths to become huge! Care must be used. Maybe a flag should be provided on whether to use autos or not?

Note: if config_from_keras is used in 'model' or 'type' mode, there is no change.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Tests

Should verify the standard tests.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label May 30, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels May 31, 2024
@jmitrevs
Copy link
Contributor Author

Added maximum precision to the keras options. In InferPrecisionTypes, _infer_common_precision now uses the maximum precision to not make the size too big. Note: for the maximum precision, basically the total width and the integer width are considered separately. The set total width is never bigger than the maximum width, and the set integer width is never bigger than the maximum integer width. No relation between the two is enforced. Also, because of #1022, there is no attempt to enforce any maximum width in _infer_sepconv_precision. After #1022, both the depthwise and pointwise convolutions use the _infer_common_precision, and _infer_sepconv_precision is no longer used.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jun 14, 2024
@jmitrevs
Copy link
Contributor Author

The test_keras_api.py::test_depthwise* and test_qkeras.py::test_qdepthwiseconv2d tests pass after #1022 is merged.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 17, 2024
@jmitrevs
Copy link
Contributor Author

I added the backend to be passed for the "config_from_*" functions. This works much better in providing all the configuration parameters one can set.

@jmitrevs
Copy link
Contributor Author

Based on discussions with other people, we decided not to add a special type propagation fallback value--the model level value is the default. We do support a maximum precision, but there is no flag to not place auto in the configuration.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 17, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 18, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 18, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 21, 2024
@jmitrevs jmitrevs added this to the v1.0.0 milestone Aug 21, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 21, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 24, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 25, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 25, 2024
@nghielme
Copy link
Contributor

nghielme commented Sep 3, 2024

Can it be extended in order to be applied to QONNX ingestion as well?

@JanFSchulte
Copy link
Contributor

I think the code in #979 infers the precision from the QONNX model, so that should be covered there. The pytorch parser however will need to be updated to use auto by default, but I would prefer to not burden this PR with this and do it as a follow-up.

@jmitrevs
Copy link
Contributor Author

jmitrevs commented Sep 3, 2024

By the way, one thing I am not too happy about is that it seems like the optimize flow has largely disappeared, because many of the optimizers benefit from running before precisions become fixed. Generally if a precision is set I have been hesitant to ignore that precision in further optimizations or doing anything that would change the numerical result. This is why I still think it may be a good idea to take infer_precision_types out of the convert flow and move it to a later stage.

@vloncar
Copy link
Contributor

vloncar commented Sep 3, 2024

Can we even do this? I thought we agreed on the approach of inferring early since the lack of precision will trip up downstream optimizers even more

@jmitrevs
Copy link
Contributor Author

jmitrevs commented Sep 3, 2024

I am not sure for the backend-specific optimizers, like the type conversions, but the type-agnostic ones don't need types, or benefit from knowing that they don't have to enforce a certain type somewhere.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 3, 2024
@nghielme
Copy link
Contributor

nghielme commented Sep 3, 2024

I think the code in #979 infers the precision from the QONNX model, so that should be covered there.

It does for weights/biases and in general for Quant nodes in the network, not for accumulators I think.

@vloncar vloncar merged commit 5d0bdb5 into fastmachinelearning:main Sep 11, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants